Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix certificate insertion #1074

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Fix certificate insertion #1074

merged 1 commit into from
Feb 19, 2024

Conversation

Keksoj
Copy link
Contributor

@Keksoj Keksoj commented Feb 13, 2024

In production, Sōzu would fail when handling the AddCertificate command, in case a certificate with a concurrent domain name was present. The candidate certificate did not overwrite the outdated certificate in the TrieNode structure that maps domain names to certificate fingerprints.

This PR operates an vast rewrite of the CertificateResolver, its types and methods, and fixes the issue. On top of that, a rare and highly unlikely bug have been prevented, in case one domain name has two valid certificates bound to it.

todo:

  • squash & co-author commits

@Keksoj Keksoj added bug tls all regarding certificates and handshakes labels Feb 13, 2024
@Keksoj Keksoj added this to the v0.16.0 milestone Feb 13, 2024
@Keksoj Keksoj self-assigned this Feb 13, 2024
@Keksoj Keksoj force-pushed the fix-certificate-insert branch 2 times, most recently from e9a137e to fa5480d Compare February 13, 2024 17:26
@Geal
Copy link
Member

Geal commented Feb 13, 2024

This looks like something that should be fixed in the control plane, not in sozu

refactor:
- simplify overriding of certificate names and expiration
- remove trait ResolveCertificate
- rename MutexWrappedCertificateResolver to MutexCertificateResolver
- rearrange error types, remove useless functions
- change signature of get_cn_and_san_attributes

fixes:
- fix concurrent certificate insert in CertificateResolver
- ensure proper removal of certificate in CertificateResolver
- fix removal of certificate with identical fingerprint

Co-Authored-By: Eloi DEMOLIS <[email protected]>
@Keksoj Keksoj force-pushed the fix-certificate-insert branch from fa5480d to 6aa45b9 Compare February 14, 2024 09:34
@Wonshtrum
Copy link
Member

Maybe the comment wasn't clear enough. If Sozu has a certificate A for domain name N, and we add a new certificate B also valid for N and A is deemed "weaker" than B (all its domains are covered by B and expire before B) then A is removed in favor of B. Which I think is the intended behavior of Sozu regardless of our control plane.

However, in this situation, the bug was that the fingerprint of A remained as the value for N in the TrieNode (because insert does nothing if the value already exists), so any TLS handshake for N would fail because domains and certificates were desynchronized.
I don't think it should be possible to put Sozu in an inconsistent state.

Or do you suggest that add certificate should never try to replace a certificate?

@Keksoj Keksoj merged commit 05f07ef into main Feb 19, 2024
21 checks passed
@Keksoj Keksoj deleted the fix-certificate-insert branch February 19, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug tls all regarding certificates and handshakes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants